Skip to content

5563 - Replace notification spec stubs with real tests#7072

Merged
compwron merged 4 commits into
rubyforgood:mainfrom
fuentesjr:5563-notification-specs
Jul 20, 2026
Merged

5563 - Replace notification spec stubs with real tests#7072
compwron merged 4 commits into
rubyforgood:mainfrom
fuentesjr:5563-notification-specs

Conversation

@fuentesjr

Copy link
Copy Markdown
Contributor

What github issue is this PR for, if any?

Part of #5563, intentionally not Resolves, since the umbrella has more stubs remaining.

What changed, and why?

Replaces the four pending "add some tests for X" stubs in spec/notifications/ with real specs (one commit per file), following the patterns in volunteer_birthday_notifier_spec.rb, reimbursement_complete_notifier_spec.rb, and emancipation_checklist_reminder_notifier_spec.rb:

  • BaseNotifier carries the bulk of the coverage: title/message/url each raise NotImplementedError on the base, plus the utility methods (read?, created_at, updated_at, created_by) that delegate to record/params, including both branches of created_by (the params[:created_by] path and the legacy params[:created_by_name] fallback)
  • FollowupNotifier covers title, url, and both message branches (with a note, joined on newlines; without, joined on spaces)
  • FollowupResolvedNotifier covers title, message, and url in both the persisted and unsaved states
  • YouthBirthdayNotifier covers title, message, and url

These characterize current behavior; no app code changed. Three # NOTE: comments flag oddities left as-is for a separate change: the NotImplementedError message string in base_notifier.rb is missing its closing quote; FollowupResolvedNotifier#message calls the private created_by_name directly while FollowupNotifier uses the public created_by; and FollowupResolvedNotifier#url's notification_id param silently drops when the notifier is unsaved (Rails omits nil query params).

One thing worth knowing for reviewers: these specs use create, not build, for any record passed into .with(...). Noticed serializes params through a jsonb column via GlobalID, and that round-trips on the first read of params, so an unsaved record raises ActiveJob::SerializationError. Only assertions that never touch params (for example a bare title) can use build.

How is this tested? (please write rspec and jest tests!) 💖💪

  • bundle exec rspec spec/notifications/base_notifier_spec.rb spec/notifications/followup_notifier_spec.rb spec/notifications/followup_resolved_notifier_spec.rb spec/notifications/youth_birthday_notifier_spec.rb gives 19 examples, 0 failures, 0 pending
  • bundle exec standardrb spec/notifications/ reports no offenses
  • No pending / skip / xit remains in the four files

Screenshots please :)

N/A, test-only change, no UI.

@github-actions github-actions Bot added 🧪 Tests Tests ruby Touches Ruby code labels Jul 18, 2026
@compwron
compwron merged commit 04c8921 into rubyforgood:main Jul 20, 2026
12 checks passed
@fuentesjr
fuentesjr deleted the 5563-notification-specs branch July 20, 2026 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ruby Touches Ruby code 🧪 Tests Tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants